Support routing matrix.to links to joinable rooms #2250
Conversation
This ends up being translated to ?server_name= in the matrix-js-sdk, although that has a bug at the time of writing. It converts `server_name: ['a', 'b']` to `?server_name=a,b` instead of `?server_name=a&server_name=b` For reference: the `viaServers` option is routed through the 'join_room' action to RoomViewStore#_joinRoom which is passed directly to the js-sdk http-api#joinRoom function. Next steps: * Fix the js-sdk parsing * Make the SDK generate matrix.to links with ?via=
turt2live
changed the title
|
This is still a WIP while I fix tests and finish testing it - just wanted to get it out there to show progress and get any early comments. |
|
Yay, this is cool! I assume once this is in and solidified an MSC could be written for the additional parameter? |
|
Yup. I want to make sure it actually solves the problem we expect it to without causing problems for people (ie: are the servers we picked actually helpful? does supplying servers actually make joins easier? etc). Once refined, an MSC will be opened assuming the refinement isn't to ditch the idea. |
In the event someone changes how the hostname parsing works.
Tests do wonders.
turt2live
changed the title
src/matrix-to.js
Outdated
| if (highestPlUser.powerLevel >= 50) candidates.push(highestPlUser.serverName); | ||
|
|
||
| const beforePopulation = candidates.length; | ||
| const maxCandidates = 3; |
There was a problem hiding this comment.
I feel like this const should be outside of the method to make its purpose even clearer
(along with a comment in case anyone is hunting through the code which thing to tweak)
| @@ -1298,7 +1298,7 @@ module.exports = React.createClass({ | |||
| // If the olmVersion is not defined then either crypto is disabled, or | |||
| // we are using a version old version of olm. We assume the former. | |||
| let olmVersionString = "<not-enabled>"; | |||
| if (olmVersion !== undefined) { | |||
| if (olmVersion) { | |||
There was a problem hiding this comment.
note: this is unrelated to the changeset here, but has a 50% chance of being the solution to the build failing in earlier commits. It seems worth keeping the change regardless of build status imo.
src/matrix-to.js
Outdated
| // room. | ||
| // | ||
| // Server 2: The next most popular server in the room (in user | ||
| // distribution). This will probably be matrix.org in most cases |
There was a problem hiding this comment.
I'd avoid mentioning matrix.org here; it's very much a misfeature that public rooms are dominated by matrix.org users today, plus this ignores usages in private federations.
This is untrue for quite a few real scenarios, including private federations and a ton of rooms.
|
This breaks room completion pills |
Fixes element-hq/element-web#7614 Regression of #2250
This was introduced in matrix-org/matrix-react-sdk#2250 but can be pulled out due to matrix-org/matrix-js-sdk#770. See #7634 for more information about the future.
This was introduced in #2250 but can be pulled out due to matrix-org/matrix-js-sdk#770. See element-hq/element-web#7634 for more information about the future.
This was introduced in matrix-org/matrix-react-sdk#2250 but can be pulled out due to #770. See element-hq/element-web#7634 for more information about the future.
Spec for [MSC1704](#1704) Reference implementations: * Original: matrix-org/matrix-react-sdk#2250 * Modern recommendations: https://github.com/matrix-org/matrix-react-sdk/blob/2ca281f6b7b735bc96945370584c5cb1b5f7e1f1/src/matrix-to.js#L29-L70 The only deviation from the original MSC is the recommendation for which servers to pick. The original MSC failed to consider server ACLs and IP addresses correctly, and during implementation it was realized that both of these cases should be handled. The core principles of the original MSC are left unaltered.
Fixes element-hq/element-web#2925 (for now)
Fixes https://github.com/vector-im/riot-web/issues/7071 (indirectly)
Fixes element-hq/element-web#7094
Fixes element-hq/element-web#1230 (can join; may not fully work. See element-hq/element-web#1230 (comment))
Requires element-hq/element-web#7552
Requires matrix-org/matrix-js-sdk#764
Please review all 3 PRs
The series of PRs here replace
browser-requestwithrequestunder the hood for the js-sdk. This is becausebrowser-requestdoesn't do any of the right things for generating a URL like/join?server_name=first.com&server_name=second.com(which is how the spec/synapse expects room joins to work when suggesting servers to join through).requeston the other hand does support doing this, albeit in an awkward way - as shown by the js-sdk PR.browser-requestis not completely removed from any of the layers due to it still being used in places. Instead of converting all of the places torequest, it seemed easier and less damaging to just leave them be. Replacing the request library in the js-sdk is already a fairly drastic change that might have unforeseen consequences. I've done some light testing ("oh good, riot still loads and I can send a message") but nothing extensive as testing every endpoint would be a nightmare.Webpack is made of fail and needs
memfsinstalled in all 3 layers. Normally we'd add this to the riot-web readme, however the react-sdk's build also fails because of it. Instead of making it angry in all the places, it's just been included in the package.json for all 3 layers. There's a comment in the webpack config for what is going on here.Note about matrix.to and introducing the new parameter:
There's no MSC associated with this yet. Technically speaking,
?via=is not a supported option for matrix.to URIs in today's environment. There's no expectation that other clients actually support this yet as this is very much a trial of how permalinking could work for now. A future MSC might make permalinking easier in Matrix, ideally making this solution a stopgap one. Provided?viameets the expectations of making links actually work, an MSC may be raised to officially include it in the spec.This is written in a way where the lack of
?viais completely okay and should therefore be backwards compatible with non-via links. Equally, a quick survey of some clients shows that adding?viashould be safe and not break other clients.